Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @jbowens and @RahulAggarwal1016)

db.go line 2605 at r3 (raw file):

// ScanStatistics returns the count of different key kinds within the lsm for a
// key span [lower, upper) as well as the number of snapshot keys.
func (d *DB) ScanStatistics(ctx context.Context, lower, upper []byte) (LSMKeyStatistics, error) {

Perhaps in a separate change, it would be nice if we could make this stop if the context is cancelled
Not sure if we want ScanInternal to do that in general (CC @itsbilal), if not we can just try to poll ctx.Done() in each callback.

That would make it possible to interrupt the query and stop the scan, which would be useful if we accidentally passed in a very big key range.

db.go line 2609 at r3 (raw file):

@jbowens maybe has an idea of how many keys per second would not be a problem for most deployments. If I had to guess, on the order of 100K to 1M keys per second?

db.go line 2612 at r3 (raw file):

	rateLimit := func() {
		fulfilled, tryAgainAfter := tb.TryToFulfill(1)

This needs to be done in a loop

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @RahulAggarwal1016)

db.go line 2609 at r3 (raw file):

I think we should rate limit on InternalKey.Size()+lazyValue.Len() so that we limit the read bandwidth. Also, I think the rate should be passed in by the user (and maybe even through the SQL builtin).

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @jbowens and @RaduBerinde)

db.go line 2609 at r3 (raw file):

for limiting based on size, what should a reasonable value for the rate and burst be (to also use in tests)?

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @jbowens and @RahulAggarwal1016)

db.go line 2678 at r5 (raw file):

type ScanStatisticsOptions struct {
	rateLimitEnabled bool
	rate             float64

I'd change to LimitBytesPerSecond int64. 0 could mean no limit.

db.go line 2690 at r5 (raw file):

	var prevKey InternalKey
	tb := tokenbucket.TokenBucket{}
	tb.Init(tokenbucket.TokensPerSecond(opts.rate), tokenbucket.Tokens(opts.burst))

I don't think "burst" needs to be configurable, we can just hardcode it to something like 1024 bytes.

db.go line 2690 at r5 (raw file):

	var prevKey InternalKey
	tb := tokenbucket.TokenBucket{}
	tb.Init(tokenbucket.TokensPerSecond(opts.rate), tokenbucket.Tokens(opts.burst))

Add a comment explaining that each "token" (roughly) corresponds to a byte that was read.

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @jbowens and @RaduBerinde)

db.go line 2612 at r3 (raw file):

fixed!

db.go line 2678 at r5 (raw file):

fixed!

db.go line 2690 at r5 (raw file):

fixed!

db.go line 2690 at r5 (raw file):

fixed!

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)

db.go line 2605 at r3 (raw file):

yeah this could be interesting, would it be like we cancel the context after we have read a certain number of bytes? Also, should I make a follow up issue for this if it is something that we want?

Reviewable status: 0 of 7 files reviewed, 7 unresolved discussions (waiting on @itsbilal, @jbowens, and @RahulAggarwal1016)

db.go line 2605 at r3 (raw file):

If you run a SQL statement in the sql shell I believe you can interrupt it and that should cancel the relevant contexts. That's the use case I have in mind.

Sure, you can file an issue. Once https://github.com/cockroachdb/tokenbucket/pull/1/files merges, we can replace the loop with WaitCtx

db.go line 2679 at r9 (raw file):

type ScanStatisticsOptions struct {
	// LimitBytesPerSecond indicates the number of bytes that are able to be read
	// per second using ScanInternal.

[nit] Mention that 0 means no limit.

Reviewable status: 0 of 7 files reviewed, 7 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)

db.go line 2679 at r9 (raw file):

fixed!

Reviewed 2 of 6 files at r8, 2 of 4 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: 5 of 7 files reviewed, 8 unresolved discussions (waiting on @itsbilal, @RaduBerinde, and @RahulAggarwal1016)

db.go line 2675 at r10 (raw file):

Let's still surface it in the output, even if we don't need it for rate limiting because it's useful information in its own right. We should add a comment describing what it is (logical, pre-compression size of keys and values read).